Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Preserve DebuggerTypeProxyAttribute classes #39126

Merged
merged 3 commits into from
Jul 14, 2020

Conversation

eerhardt
Copy link
Member

Also, ensure the test app assembly is linked, and not copied during trimming tests, which was a bug in our test infrastructure.

Fix #37307

Note that this will actually make apps larger now that we are preserving the DebuggerTypeProxyAttribute classes by default.

Once #39000 is merged, we can add the DebuggerTypeProxyAttribute (and other debugging related attributes) to be removed when the feature switch System.Diagnostics.Debugger.IsSupported is false.

cc @stephentoub @MichalStrehovsky @vitek-karas

Also, ensure the test app assembly is linked, and not copied during trimming tests, which was a bug in our test infrastructure.

Fix dotnet#37307
@Dotnet-GitSync-Bot
Copy link
Collaborator

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@ghost
Copy link

ghost commented Jul 10, 2020

Tagging subscribers to this area: @tommcdon, @krwq
Notify danmosemsft if you want to be subscribed.

{
ProxyTypeName = typeName;
}

// The Proxy is only invoked by the debugger, so it needs to have its
// members preserved
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this attribute mean when used on a get-only auto-prop?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Linker will auto-propagate it to the backing field - and the field assignment in .ctor will see it and act accordingly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. What does it do that the same attribute on the ctor args doesn't achieve?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Linker doesn't auto-propagate annotations across method bodies. So the store to the backing field in the .ctor will NOT annotate the backing field. And so if something somewhere consumes the backing field (or rather the property getter), it would not get the annotation. The best way to think about this is sort of backwards:

  • We annotate the property getter because the type should fulfill such requirements (this specific example is a bit weird since the requirement doesn't come from managed code)
  • Linker will auto-propagate that annotation to the backing field (this is the only auto-propagation we do, mostly as it's relatively easy and it doesn't require many attribute in one place)
  • Now the store in the .ctor will generate a warning as it's assigning unannotated value to an annotated field
  • So we annotate the .ctor argument
  • This specific .ctor argument is pretty much only used with statically known types (that is typeof(TypeName) expressions) - so the linker will auto-apply the annotation to the statically known type - the chain ends here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But why does it matter that the field is annotated? There only way to construct this type is with the two ctors. Both take a type (one as a string) input, and both are attributed the same way. When a type is passed to either ctor, the linker should see that and keep its members accordingly.

What is an example where just annotating the ctor arguments here is insufficient? What does also attributing the backing field enable? That's what I'm missing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They'd get a warning, but would the linker have actually removed things they may be depending on from the type?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, annotating the constructor is all that's required to prevent the type from being linked away.

Annotating the property allows the linker analysis to prove that the code is correct. We want to drive the warnings baselines in #39133 to zero.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, not pushing back, eliminating the warnings is important. But it is confusing that the annotation required to make it correct/safe is insufficient to make it warning-free, and that there's no distinction between the attributes used for those.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This specific case is not a good example of how the annotations should work. But let's assume the debugger is somewhere in managed code in the app - so something has a reference to the ProxyTypeName property and presumably it does something like:

var propertiesToShow = Type.GetType(attr.ProxyTypeName).GetProperties();

When linker sees that it goes (basically backwards):

  • I need all properties on the type returned by Type.GetType (cause there's a call to GetProperties() on it)
  • So it means that the type name coming to Type.GetType must be a type with all properties
  • So it means I need the attr.ProxyTypeName to be a type name with all properties -> it should have at least DynamicallyAccessedMembers(DynamicallyAccessedMemberType.PublicProperties).
  • If it doesn't -> warn
  • So we go and annotate the ProxyTypeName with that annotation
  • Now running the linker again it will not warn on the usage above anymore, but it will warn in the attribute's .ctor since we're trying to assign an unannotated string to the backing field of ProxyTypeName
  • So we go and fix that by annotating the .ctor argument.
  • Running linker again it will not warn in the .ctor itself, but it now requires all the callers to fulfill that annotation. But since this is an attribute all callers provide constant strings as the argument - for this linker knows what to do, it finds the type of that name, keeps it and then also keeps all of its properties.

So for the application to work - only the last step is needed, for which only the annotation on the .ctor argument is needed. But that would not be enough for linker to figure out that the code using it is correct - and so it would have to warn (it doesn't do cross method analysis). So the other annotations, like the one on the property, are there to help linker prove that the code is correct and that it will work.

The other advantage is that if later on we change the usage and now call for example .GetProperties(BindingFlags.NonPublic), linker would suddenly warn, because the annotations don't cover private properties anymore and the app may break. Without the annotations flowing through everything it would have no way to tell that there's something wrong.

The reason why the annotations are the same everywhere is that they always mean the same thing - it's a requirement on the value which is passed in. That can be fulfilled either by:

  • passing in value with same or "bigger" annotation
  • passing in statically known value which the linker can actually find - and then it can make sure that type in question fulfills the requirements

Since the method/property itself doesn't which one of the two cases it will be, it applies the annotation - always the same one.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the details.

@eerhardt
Copy link
Member Author

CI failure is #39291. Merging.

@eerhardt eerhardt merged commit 142021c into dotnet:master Jul 14, 2020
@eerhardt eerhardt deleted the DebugProxyAttribute branch July 14, 2020 20:45
@karelz karelz added this to the 5.0.0 milestone Aug 18, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DebuggerTypeProxy types are empty after linking
8 participants